-
Notifications
You must be signed in to change notification settings - Fork 353
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add api level check to request Notification permission #5221
Add api level check to request Notification permission #5221
Conversation
DROID-386 Notification permission api check
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use proper formatting in the description (punctuation, capitalization etc). Also, this PR is about fixing the issue rather than suppressing it, right?
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @sabercodic)
android/lib/common/src/main/kotlin/net/mullvad/mullvadvpn/lib/common/util/SdkUtils.kt
line 27 at r1 (raw file):
} fun Context.notificationPermissionIsNotGranted(): Boolean {
Why do we need to (more or less) duplicate the above function? And can you elaborate on why this fixes the issue. Seems like it might be more of a issue with the lint tool itself.
1b52f0a
to
d36ce06
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @sabercodic)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/MainActivity.kt
line 355 at r2 (raw file):
private fun checkForNotificationPermission() { if (!isNotificationPermissionGranted().not()) {
Why is there a double negative (! .. .not()
)?
Code quote:
!isNotificationPermissionGranted().not()
90333f4
to
c3a4f91
Compare
274cab8
to
90a2b63
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @sabercodic)
android/lib/common/src/main/kotlin/net/mullvad/mullvadvpn/lib/common/util/SdkUtils.kt
line 28 at r4 (raw file):
fun getNotificationPermissionResource(): String? { return if ((Build.VERSION.SDK_INT > Build.VERSION_CODES.TIRAMISU)) {
Redundant parenthesis?
90a2b63
to
0998e0a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @albin-mullvad and @Pururun)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/MainActivity.kt
line 355 at r2 (raw file):
Previously, albin-mullvad wrote…
Why is there a double negative (
! .. .not()
)?
Done.
android/lib/common/src/main/kotlin/net/mullvad/mullvadvpn/lib/common/util/SdkUtils.kt
line 27 at r1 (raw file):
Previously, albin-mullvad wrote…
Why do we need to (more or less) duplicate the above function? And can you elaborate on why this fixes the issue. Seems like it might be more of a issue with the lint tool itself.
Done.
android/lib/common/src/main/kotlin/net/mullvad/mullvadvpn/lib/common/util/SdkUtils.kt
line 28 at r4 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Redundant parenthesis?
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @albin-mullvad)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @albin-mullvad and @sabercodic)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/MainActivity.kt
line 357 at r5 (raw file):
private fun checkForNotificationPermission() { if (isNotificationPermissionGranted().not()) { getNotificationPermissionResource()?.let {
Looks kind of weird to be that we do the requestNotificationPermissionLauncher
thingy given that we get a resource.
I think it would be more clear to just say:
if (Build.VERSION.SDK_INT > Build.VERSION_CODES.TIRAMISU) {
requestNotificationPermissionLauncher.launch(Manifest.permission.POST_NOTIFICATIONS)
}
or maybe it can also be more integrated into isNotificationPermission granted? Because that should also require the same API level?
32c62f7
to
0e8d279
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @albin-mullvad and @sabercodic)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 3 files at r4, 1 of 1 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @sabercodic)
-- commits
line 2 at r7:
Please clarify in the commit and PR title what lint issue this fixes. "InlinedApi" doesn't say much.
Code quote:
Fix InlinedApi lint issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @albin-mullvad)
Previously, albin-mullvad wrote…
Please clarify in the commit and PR title what lint issue this fixes. "InlinedApi" doesn't say much.
Done.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/MainActivity.kt
line 357 at r5 (raw file):
Previously, Rawa (David Göransson) wrote…
Looks kind of weird to be that we do the
requestNotificationPermissionLauncher
thingy given that we get a resource.I think it would be more clear to just say:
if (Build.VERSION.SDK_INT > Build.VERSION_CODES.TIRAMISU) { requestNotificationPermissionLauncher.launch(Manifest.permission.POST_NOTIFICATIONS) }
or maybe it can also be more integrated into isNotificationPermission granted? Because that should also require the same API level?
We try to keep all the sdk check inside SDKUtil, @albin-mullvad what is your idea here?
0e8d279
to
f2d1b95
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Rawa and @sabercodic)
Previously, sabercodic wrote…
Done.
The PR title is still the same, I asked for both to be updated
android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/MainActivity.kt
line 357 at r5 (raw file):
Previously, sabercodic wrote…
We try to keep all the sdk check inside SDKUtil, @albin-mullvad what is your idea here?
After looking a bit at the documentation, I think that we would want to make use of the ChecksSdkIntAtLeast
annotation (https://developer.android.com/reference/androidx/annotation/ChecksSdkIntAtLeast). I tried it out, but for some reason it doesn't seem to work. Perhaps due to the classes being part of different modules. Here's how it could look: 0425a9b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @albin-mullvad and @Rawa)
Previously, albin-mullvad wrote…
The PR title is still the same, I asked for both to be updated
Done.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/MainActivity.kt
line 357 at r5 (raw file):
Previously, albin-mullvad wrote…
After looking a bit at the documentation, I think that we would want to make use of the
ChecksSdkIntAtLeast
annotation (https://developer.android.com/reference/androidx/annotation/ChecksSdkIntAtLeast). I tried it out, but for some reason it doesn't seem to work. Perhaps due to the classes being part of different modules. Here's how it could look: 0425a9b
Is it ok to add new dependencies to basic modules?
f6ea463
to
31f5672
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @albin-mullvad, @Pururun, and @sabercodic)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/MainActivity.kt
line 357 at r5 (raw file):
Previously, sabercodic wrote…
Is it ok to add new dependencies to basic modules?
I don't see why it would be a problem if it is a dependency we need and it seems reliable. What dependency are you thinking about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @albin-mullvad, @Pururun, and @Rawa)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/MainActivity.kt
line 357 at r5 (raw file):
Previously, Rawa (David Göransson) wrote…
I don't see why it would be a problem if it is a dependency we need and it seems reliable. What dependency are you thinking about?
@albin-mullvad I tested this by creating temp util file in app module and it worked fine but when I moved it to other module, as you said it was not working.
@Rawa I was talking about adding ChecksSdkIntAtLeast
(androidx.annotation:annotation-jvm) to the 'lib' module
31f5672
to
89a9d86
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Rawa)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/MainActivity.kt
line 357 at r5 (raw file):
Previously, sabercodic wrote…
@albin-mullvad I tested this by creating temp util file in app module and it worked fine but when I moved it to other module, as you said it was not working.
@Rawa I was talking about adding
ChecksSdkIntAtLeast
(androidx.annotation:annotation-jvm) to the 'lib' module
It's completely fine to add that dependency, especially since it's part of androidx. Unfortunate that it doesn't seem to work though. @sabercodic can see if there's a way to make it work?
45229bc
to
99e44c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dismissed @Rawa and @albin-mullvad from a discussion.
Reviewable status: 0 of 8 files reviewed, all discussions resolved
android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/MainActivity.kt
line 357 at r5 (raw file):
Previously, albin-mullvad wrote…
It's completely fine to add that dependency, especially since it's part of androidx. Unfortunate that it doesn't seem to work though. @sabercodic can see if there's a way to make it work?
I've inverted the logic and it seems to work better with the AGP linting now.
99e44c2
to
79e658f
Compare
79e658f
to
0dd07d9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 4 files at r11, 2 of 6 files at r13, 4 of 4 files at r14, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sabercodic)
android/lib/common/build.gradle.kts
line 32 at r14 (raw file):
implementation(project(Dependencies.Mullvad.talpidLib)) implementation(Dependencies.AndroidX.appcompat)
We should probably have a discussion/create a meeting in regards to how we want to structure the app and what the different modules should do. But for now I guess this is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved
android/lib/common/build.gradle.kts
line 32 at r14 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
We should probably have a discussion/create a meeting in regards to how we want to structure the app and what the different modules should do. But for now I guess this is fine.
Yes, will do that!
Fix lint warning related to notification permission minimum api level.
This change is